-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Default to add '-wdir' arguments #351
Conversation
Is the MVIPICH wrapper still used? If so, I could revert the changes and only add ‘-wdir’ arguments if mpi_module does not start with ‘mvapich’ rather than previously which was checking mpi_module starts with |
Looks like MPICH supports https://www.mpich.org/static/docs/v3.4.x/www1/mpiexec.html See #350 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More feedback!
payu/fsops.py
Outdated
# NOTE: ldd <binary> | ||
|
||
# Example 1: | ||
# libmpi.so.40 => /apps/openmpi/4.0.2/lib/libmpi.so.40 (0x00007fa493665000) | ||
# libmpi_usempif08_Intel.so.40 => /apps/openmpi/4.0.2/lib/libmpi_usempif08_Intel.so.40 (0x00007fa492a7c000) | ||
# libmpi_usempi_ignore_tkr_Intel.so.40 => /apps/openmpi/4.0.2/lib/libmpi_usempi_ignore_tkr_Intel.so.40 (0x00007fa492863000) | ||
# libmpi_mpifh_Intel.so.40 => /apps/openmpi/4.0.2/lib/libmpi_mpifh_Intel.so.40 (0x00007fa4925cb000) | ||
|
||
# Example 2: | ||
# libmpi_usempif08.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi_usempif08.so.40 (0x00007f12671c0000) | ||
# libmpi_usempi_ignore_tkr.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi_usempi_ignore_tkr.so.40 (0x00007f1266fb4000) | ||
# libmpi_mpifh.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi_mpifh.so.40 (0x00007f1266d44000) | ||
# libmpi.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi.so.40 (0x00007f12667c3000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion these examples should be moved into a test of required_libs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've separating the parsing of ldd output into a separate file so its simpler to test. The other option could be to mock out the ldd call?
250303a
to
279fedb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed you could mock ldd
as an alternative to storing the output in a file, but seems a lot of hassle and I kind of like the resulting design as it isolates functionality, which is not only good for your testing, but allows someone else to call a function like parse_ldd_output
independently of running payu
, which can help with checking for errors, or ensuring the code is doing what you expect.
# Our MVAPICH wrapper does not support working directories | ||
if mpi_module.startswith('mvapich'): | ||
curdir = os.getcwd() | ||
os.chdir(self.work_path) | ||
else: | ||
curdir = None | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to leave a comment to say deleting this relates directly to #350.
This just can't work with submodels and there is no attempt to test for that, nor do we want to support something that is so limited, so it is being removed.
Co-authored-by: Aidan Heerdegen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to merge.
Default to add '-wdir' arguments
Change logic in experiments to default to adding
-wdir
to the arguments given to mpirun.Should close #341 and close #350